-
Notifications
You must be signed in to change notification settings - Fork 24.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Wildcard field names in highlighting should only return fields that can be highlighted #11364
Conversation
highlightFields.put(highlightField.name(), highlightField); | ||
} | ||
} catch (FetchPhaseExecutionException e) { | ||
if (fieldNameContainsWildcards && e.getCause() instanceof BytesRefHash.MaxBytesLengthExceededException) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a plain highlighter only problem? Not sure, but if so maybe we could handle it in the PlainHighlighter directly rather than in the HighlightPhase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes makes sense. I'll make it so that this Exception will always be caught even if no wildcards are used in the field name.
I left some comments ;) |
@javanna thanks a lot for the review! addressed all comments. want to have another look? |
if (useFastVectorHighlighter) { | ||
highlighterType = "fvh"; | ||
} else if (fieldMapper.fieldType().indexOptions() == IndexOptions.DOCS_AND_FREQS_AND_POSITIONS_AND_OFFSETS) { | ||
} else if (highlighters.get("postings").canHighlight(fieldMapper)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe overkill, but is it worth keeping a list of highlighters ordered by precedence instead? it could even allow plugins to provide their own precedence, but that's maybe too much? I am not sure, just asking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If anyone actually needs that maybe have another issue for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alright, that makes sense, apart from the feature for plugins, it might still be nicer to have a sorted list and go through the available highlighters, call canHighlight and highlight using the first one that returns true? I mean rather than getting them by name one by one? Do you like this idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does seem a bit cleaner to use a list.
For what its worth my highlighter would just return true
from canHighlight because its defaults let it pick its hit detection strategy based on what is in the mapping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, added a new commit that adds a list. is that what you meant?
looks great, left a few more comments, I think this should marked as breaking as it breaks plugins that plug in custom highlighters, maybe @nik9000 would like to have a look too? |
Just did. I think its fine. I agree with your point about a sorted list being easier to read. I also think its worth moving any concerns about highlighter plugins registering themselves to the list to another issue. At this point I don't think that is really an important feature. |
sortedListBuilder.add(highlighters.get("fvh")); | ||
sortedListBuilder.add(highlighters.get("postings")); | ||
sortedListBuilder.add(highlighters.get("plain")); | ||
for (Map.Entry highlighter : highlighters.entrySet()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably isn't required - other highlighters can just be called out in the request if you want them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed we can keep custom highlighters out of the loop for now, anyways they can currently be used only by referring to them with highlighter_type parameter if I remember correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right - you need to call them out explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, removed them again
ok, changed that now to use a list. want to take another look? |
|
||
@Inject | ||
public HighlightPhase(Settings settings, Highlighters highlighters) { | ||
super(settings); | ||
this.highlighters = highlighters; | ||
// TODO: would be nice if each highlighter would have a precedence and we could just sort here by this value | ||
ImmutableList.Builder sortedListBuilder = ImmutableList.builder(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I meant, yeah. I'd have made it a private static final ImmutableList<String>
instead of Immutable<Highlighter>
but it doesn't make much difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 that is what I would do too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, done. But not static, that seems wrong and highlighters is not static either.
0ad1161
to
92eaf98
Compare
all done. want to take another look? |
|
||
@Inject | ||
public HighlightPhase(Settings settings, Highlighters highlighters) { | ||
super(settings); | ||
this.highlighters = highlighters; | ||
// TODO: would be nice if each highlighter would have a precedence and we could just sort here by this value | ||
// we could then also add custom highlighters to this list and they could be selected automatically depending on precendence |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you remove this TODO? I'm not sure we are going to implement this after all, nobody needs it for now :)
done another review, few more comments :) |
2d27496
to
fe9810a
Compare
pushed another commit |
@@ -63,7 +63,7 @@ public HighlightField highlight(HighlighterContext highlighterContext) { | |||
FetchSubPhase.HitContext hitContext = highlighterContext.hitContext; | |||
FieldMapper mapper = highlighterContext.mapper; | |||
|
|||
if (!(mapper.fieldType().storeTermVectors() && mapper.fieldType().storeTermVectorOffsets() && mapper.fieldType().storeTermVectorPositions())) { | |||
if (canHighlight(mapper) == false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!canHighlight(mapper)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the == false is done on purpose to make these comparisons more explicit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get the comment usually the other way round with the argument that '== false' is much easier to read and a '!' is easy to miss so I'd rather leave it this way.
LGTM besides the two very minor comments I left, thanks for taking care of this @brwe ! |
I left another distinct minor comment but yeah, its great! This'll make highlighting so much less blowy-upy. |
a79eca8
to
edcdf7a
Compare
addressed all comments |
LGTM |
…d contains wildcards When we highlight on fields using wildcards then fields might match that cannot be highlighted by the specified highlighter. The whole search request then failed. Instead, check that the field can be highlighted and ignore the field if it can't. In addition ignore the exception thrown by plain highlighter if a field conatins terms larger than 32766. closes elastic#9881
edcdf7a
to
3761054
Compare
Wildcard field names in highlighting should only return fields that can be highlighted
When we highlight on fields using wildcards then fields might match that cannot
be highlighted by the specified highlighter. The whole search request then
failed. Instead, check that the field can be highlighted and ignore the field
if it can't.
In addition ignore the exception thrown by plain highlighter if a field conatins
terms larger than 32766.
closes #9881